Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify Hover-Behavior Between Light and Dark Theme #1645

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

BeckerWdf
Copy link
Contributor

In the "Dark" theme the background color of unselected tabs in CTabFolders changes color when user hovers over them with the mouse cursor. This helps a lot to see where an inactive tab starts and ends.

This change adds the same behavior also for the light theme.

@BeckerWdf
Copy link
Contributor Author

BeckerWdf commented Feb 6, 2024

This is how the dark theme already looks like:
Screen Recording dark

This how this looks like on macOS before this change
Screen Recording before

This how this looks like on macOS after this change

Screen Recording after

@BeckerWdf
Copy link
Contributor Author

I think this also helps a lot for user that have the "Hide icon" setting on the "Appearance" preference page (#1071) turned on.

Copy link
Contributor

github-actions bot commented Feb 6, 2024

Test Results

   917 files  ±0     917 suites  ±0   36m 53s ⏱️ - 1m 36s
 7 434 tests ±0   7 282 ✅  - 1  150 💤 ±0  2 ❌ +1 
21 878 runs  ±0  21 493 ✅  - 1  383 💤 ±0  2 ❌ +1 

For more details on these failures, see this check.

Results for commit 271ef9c. ± Comparison against base commit 1472375.

♻️ This comment has been updated with latest results.

@BeckerWdf
Copy link
Contributor Author

@HeikoKlare and @thomasritter: What do you think?

@HeikoKlare
Copy link
Contributor

I agree that the proposed behavior is desirable. On Windows, the behavior is already as expected without this PR: hovering over unselected tab items (no matter if the tab folder has focus or not) highlights them. In the following screenshot, I hover over the "Declaration" tab. Note that the color is also lighter than the one selected in this PR:
image

Unfortunately, with this change, it does now work anymore on Windows. Behavior is then as follows: hovering over a tab item only highlights it if the tab folder does not have focus. And in that case, the highlight color is notably darker than in the current implementation. Again, I hover over the "Declaration" tab:
image

So there seem to be different theme colors and/or different CSS interpretations at least in the Mac and Windows implementations.

@BeckerWdf
Copy link
Contributor Author

I agree that the proposed behavior is desirable. On Windows, the behavior is already as expected without this PR: hovering over unselected tab items (no matter if the tab folder has focus or not) highlights them. In the following screenshot, I hover over the "Declaration" tab. Note that the color is also lighter than the one selected in this PR: image

Can you tell me what color definition is used here?

Unfortunately, with this change, it does now work anymore on Windows. Behavior is then as follows: hovering over a tab item only highlights it if the tab folder does not have focus. And in that case, the highlight color is notably darker than in the current implementation. Again, I hover over the "Declaration" tab: image

So there seem to be different theme colors and/or different CSS interpretations at least in the Mac and Windows implementations.

That's strange.
For windows light the following CSS file is used: e4_default_win.css. This includes /css/e4_basestyle.css, which in turn includes /css/light/e4-light_tabstyle.css.
For mac light the following CSS file is used: e4_default_mac.css. This includes /css/e4_basestyle.css, which is the same as for windows.

So the difference has to be in the CSS definitons inside e4_default_win.css. And in this file we have (besides some ColorDefinitions) only this:

.MPartStack {
	swt-simple: true;
}

CTabFolder.MArea {
	background-color: #F0F0F0;
	swt-selected-tab-fill: #F0F0F0;
	swt-unselected-tabs-color: #F0F0F0;
	swt-outer-keyline-color: #F0F0F0;
	swt-inner-keyline-color: #F0F0F0;
	swt-tab-outline: #F0F0F0;
}

CTabFolder Canvas {
	background-color: #F8F8F8;
}

Which of these properties causes the hover-behaviour on windows?

@HeikoKlare
Copy link
Contributor

I changed all of them, but none had any effect on the hover behavior. Probaby the hover behavior is simply the default behavior of the OS widget.

But I now found why your proposal "breaks" behavior on Windows: the highlight color you chose is exactly the same as the background color of a tab folder that has focus. You can see that in the following screenshot: in both cases, I hover over the "Declaration" tab. The tab folder at the top has focus, while the one at the bottom has not.

image

Using the background color for active rather than inactive tabs would fix that. That is actually the color that Windows already uses by default, so it would change nothing on Windows compared to existing behavior.

@HeikoKlare
Copy link
Contributor

I tested on Windows and Linux and both look good now 👍 That means: the patch does not change any behavior for these systems, as both Win32 and GTK seem to have the hover highlighting behavior as default.

Just one thing I found when taking another look at the CSS configurations: usually there are color keys "start" and "end", probably in case the theme defines some gradient? E.g.

'#org-eclipse-ui-workbench-ACTIVE_TAB_BG_START' '#org-eclipse-ui-workbench-ACTIVE_TAB_BG_END' 100% 100%;

Maybe the added configuration should also consider that, even though gradients are rather uncommon now?

@BeckerWdf
Copy link
Contributor Author

I tested on Windows and Linux and both look good now 👍 That means: the patch does not change any behavior for these systems, as both Win32 and GTK seem to have the hover highlighting behavior as default.

Just one thing I found when taking another look at the CSS configurations: usually there are color keys "start" and "end", probably in case the theme defines some gradient? E.g.

'#org-eclipse-ui-workbench-ACTIVE_TAB_BG_START' '#org-eclipse-ui-workbench-ACTIVE_TAB_BG_END' 100% 100%;

Maybe the added configuration should also consider that, even though gradients are rather uncommon now?

I also thought about the gradient and explicitly opted against it. It's uncommon in todays UIs. Should we merge this as is?

@vogella
Copy link
Contributor

vogella commented Feb 6, 2024

Comment to the same start and end gradient colour: IIRC the CSS engine only supported gradients hence to remove it I had to set both to the same value. It would better if the css API supports a single colour as we could simplify our CSS files but I never found the time to investigate/ implement this.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side, this is ready to merge.

@BeckerWdf
Copy link
Contributor Author

Why is the CI build failing now?
Remote call to JNLP4-connect connection from 10.40.66.231/10.40.66.231:56210

@HeikoKlare
Copy link
Contributor

Build is failing because of
META-INF/MANIFEST.MF:0 The re-exported type org.eclipse.swt.ole.win32.OLE has been removed from org.eclipse.jface_3.32.0 etc.
These errors also occur in master builds: https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/335/console

This is an SWT issue (I will inspect if that's covered by one of the issues processed today).

@BeckerWdf
Copy link
Contributor Author

Build is failing because of META-INF/MANIFEST.MF:0 The re-exported type org.eclipse.swt.ole.win32.OLE has been removed from org.eclipse.jface_3.32.0 etc. These errors also occur in master builds: https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/335/console

This is an SWT issue (I will inspect if that's covered by one of the issues processed today).

Thanks for looking into it.

@BeckerWdf
Copy link
Contributor Author

Comment to the same start and end gradient colour: IIRC the CSS engine only supported gradients hence to remove it I had to set both to the same value. It would better if the css API supports a single colour as we could simplify our CSS files but I never found the time to investigate/ implement this.

the property 'swt-unselected-hot-tab-color-background' doesn't support gradients but only a single color
see:

	@Override
	protected void applyCSSProperty(Control control, String property,
			CSSValue value, String pseudo, CSSEngine engine) throws Exception {
		if (!(control instanceof CTabFolder) || value.getCssValueType() != CSSValue.CSS_PRIMITIVE_VALUE) {
			return;
		}

		if (UNSELECTED_HOT_TAB_COLOR_BACKGROUND.equals(property)) {
			Color newColor = (Color) engine.convert(value, Color.class, control.getDisplay());
			CTabFolderRenderer renderer = ((CTabFolder) control).getRenderer();
			if (renderer instanceof ICTabRendering) {
				((ICTabRendering) renderer).setUnselectedHotTabsColorBackground(newColor);
			}
		}
	}

@iloveeclipse
Copy link
Member

Build is failing because of META-INF/MANIFEST.MF:0 The re-exported type org.eclipse.swt.ole.win32.OLE has been removed from org.eclipse.jface_3.32.0 etc. These errors also occur in master builds: https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/335/console

This is an SWT issue (I will inspect if that's covered by one of the issues processed today).

I've reported #1654

@BeckerWdf
Copy link
Contributor Author

Build is failing because of META-INF/MANIFEST.MF:0 The re-exported type org.eclipse.swt.ole.win32.OLE has been removed from org.eclipse.jface_3.32.0 etc. These errors also occur in master builds: https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/335/console
This is an SWT issue (I will inspect if that's covered by one of the issues processed today).

I've reported #1654

Thanks.

In the "Dark" theme the background color of unselected tabs in
CTabFolders changes color when user hovers over them with the mouse
cursor. This helps a lot to see where an inactive tab starts and ends.

This change adds the same behavior also for the light theme.
@BeckerWdf
Copy link
Contributor Author

The two failing tests are already reported in:
#1005
#1427

@BeckerWdf BeckerWdf merged commit fd826d3 into eclipse-platform:master Feb 13, 2024
13 of 16 checks passed
@BeckerWdf BeckerWdf deleted the tab_hover branch February 13, 2024 11:33
BeckerWdf added a commit to BeckerWdf/www.eclipse.org-eclipse that referenced this pull request Feb 13, 2024
BeckerWdf added a commit to eclipse-platform/www.eclipse.org-eclipse that referenced this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants